Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Indexer fails to identify continuation preceded by newline #10351 #10354

Merged
merged 1 commit into from
Mar 12, 2024
Merged

Fix Indexer fails to identify continuation preceded by newline #10351 #10354

merged 1 commit into from
Mar 12, 2024
+28 −7

Conversation

augustelalande
Copy link
Contributor

Summary

Fixes #10351

It seems the bug was caused by this section of code

ruff/crates/ruff_python_index/src/indexer.rs

Lines 55 to 58 in b669306

// Newlines after a newline never form a continuation.
if !matches!(prev_token, Some(Tok::Newline | Tok::NonLogicalNewline)) {
continuation_lines.push(line_start);
}

It's true that newline tokens cannot be immediately followed by line continuations, but only outside parentheses. e.g. the exception

(
    1
    \
    + 2)

But why was this check put there in the first place? Is it guarding against something else?

Test Plan

New test was added to indexer

@github-actions GitHub Actions
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@charliermarsh
Copy link
Member

Can you find anything via git blame about why this is here?

@charliermarsh
Copy link
Member

(It's possible that something changed in the lexer over time that made this not necessary.)

@augustelalande
Copy link
Contributor Author

This was the original implementation

ruff/src/source_code/indexer.rs

Lines 22 to 48 in 3791ca7

impl From<&[LexResult]> for Indexer {
fn from(lxr: &[LexResult]) -> Self {
let mut commented_lines = Vec::new();
let mut continuation_lines = Vec::new();
let mut prev: Option<(&Location, &Tok, &Location)> = None;
for (start, tok, end) in lxr.iter().flatten() {
if matches!(tok, Tok::Comment(_)) {
commented_lines.push(start.row());
}
if let Some((.., prev_tok, prev_end)) = prev {
if !matches!(
prev_tok,
Tok::Newline | Tok::NonLogicalNewline | Tok::Comment(..)
) {
for line in prev_end.row()..start.row() {
continuation_lines.push(line);
}
}
}
prev = Some((start, tok, end));
}
Self {
commented_lines,
continuation_lines,
}
}
}

which had slightly different logic and needed the check to avoid flagging every newline. The checks just got kept through the edits.

Note: This implementation would also have failed to parse the code in question correctly.

@charliermarsh
Copy link
Member

Okay, I think it was this change, long ago: RustPython/Parser#27

@charliermarsh charliermarsh merged commit dacec73 into astral-sh:main Mar 12, 2024
17 checks passed
@charliermarsh charliermarsh added the bug Something isn't working label Mar 12, 2024
@augustelalande augustelalande deleted the indexer-bug branch March 12, 2024 04:54
@BrewTestBot BrewTestBot mentioned this pull request Mar 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Indexer fails to identify continuation preceded by newline
2 participants
@augustelalande @charliermarsh